Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make sure we remove all document listeners on dispose. #1475

Closed

Conversation

mynameisstephen
Copy link

The slider is not removing document event listeners on dispose. I have a feeling that there are other instances within other components.

@heff
Copy link
Member

heff commented Sep 4, 2014

The document listeners are removed on the slider's mouseup event, so this would fix the case that the player is disposed of in the middle of sliding a slider. Seems unlikely but not impossible. Was there a specific use case where you saw this happen, or are you finding the listeners aren't being removed on mouseup as expected? Or is this tightening things up?

@mynameisstephen
Copy link
Author

Essentially I have a rudimentary playlist which plays a video and on complete disposes the video player and creates a new one with the next video. I get a null error when seeking to the end of the video causing it to trigger the complete event and than dispose the video player.

TypeError: element is null

vjs.removeClass@http://dev.local/static/js/video.dev.js:996:1
vjs.Component.prototype.removeClass@http://dev.local/static/js/video.dev.js:2288:3
vjs.Slider.prototype.onMouseUp@http://dev.local/static/js/video.dev.js:2749:3
vjs.SeekBar.prototype.onMouseUp@http://dev.local/static/js/video.dev.js:5232:3
vjs.bind/ret@http://dev.local/static/js/video.dev.js:868:5
vjs.on/data.dispatcher@http://dev.local/static/js/video.dev.js:343:13

#1476 is another example of event listeners staying alive. Disposing a video player while in full screen mode eventually causes a null exception.

@heff
Copy link
Member

heff commented Sep 29, 2014

This one is pretty simple so I'll pull it in for the next release. Ideally we'll move towards what's described in #1544 to handle this situation.

@heff
Copy link
Member

heff commented Sep 29, 2014

Merged in. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants